-
Notifications
You must be signed in to change notification settings - Fork 4
switch all hpge processors to use ak.Array and unit handelling #121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
not yet. the parameters from the ak arrays are not stored into the lgdo for now... Also a = ak.Array(np.array([1,2,4]), attrs={"unit": "m"})
ak.parameters(a) # {}
a = ak.Array(np.array([1,2,4]))
a = ak.with_parameter(a, "unit", "m")
ak.parameters(a) # {'unit': 'm'} |
|
I do not understand works nicely |
|
I think this is now fixed, but we should think about having much testing of the units handling. Eg for every processor:
This will take time |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 69.72% 70.04% +0.31%
==========================================
Files 33 33
Lines 2583 2597 +14
==========================================
+ Hits 1801 1819 +18
+ Misses 782 778 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I think this is good to go and can be merged now, any objection? |
|
The Code with `.attrs` is still not the right way I guess? (If I remember right, we continued the discussion elsewhere on Slack? - the attrs are not preserved in all cases, only parameters are)
Am 1. November 2025 11:09:38 MEZ schrieb Luigi Pertoldi ***@***.***>:
…gipert left a comment (legend-exp/reboost#121)
I think this is good to go and can be merged now, any objection?
|
|
Ok let's wait until we properly test this |
|
This is not ready yet, it's a rather large change and needs careful testing.
Sent from Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Luigi Pertoldi ***@***.***>
Sent: Saturday, November 1, 2025 11:52:19 AM
To: legend-exp/reboost ***@***.***>
Cc: Dixon, Toby ***@***.***>; Author ***@***.***>
Subject: Re: [legend-exp/reboost] switch all hpge processors to use ak.Array and unit handelling (PR #121)
⚠ Caution: External sender
[https://avatars.githubusercontent.com/u/20358192?s=20&v=4]gipert left a comment (legend-exp/reboost#121)<#121 (comment)>
Ok let's wait until we properly test this
—
Reply to this email directly, view it on GitHub<#121 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANSET4YYJI6IVW4ECBHD77T32SGGHAVCNFSM6AAAAACHMDCPKKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTINZWGIYDANRVHE>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
We should try to make some progress here... |
|
Since we break the interface, we should also fix things like #136 before releasing a new version |
|
I think we need to do some more work to support units in reshaping? |
|
Related: legend-exp/legend-pydataobj#207 |
|
can you also add some docs about the new conventions? |
the old docs are here: reboost/docs/source/manual/processors.md Lines 53 to 59 in 0f7fc07
|
Co-authored-by: Manuel Huber <ManuelHu@users.noreply.github.com>
Still some things are ad-hoc. My general idea is to have the public API functions convert the units to some internal set to be used in processors.
What I am not sure of is if this will really get used with the procesing chain / config (@ManuelHu ?)
Warning: This PR contains quite some API changes to switch to the new conventions